Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: some type reflection improvements #22350

Merged
merged 1 commit into from
Jul 10, 2017
Merged

RFC: some type reflection improvements #22350

merged 1 commit into from
Jul 10, 2017

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Jun 13, 2017

The definition of nfields has been bothering me, since it's a bit ambiguous whether it's telling you the number of fields of the object you pass, or if the object you pass is a type, the number of fields of instances of that type. This leads to some questionable and/or wrong results like:

julia> nfields(Tuple{T,T,T} where T)
2

(Since a UnionAll has 2 fields.)

I think there are two three possible approaches:

  1. Make nfields refer only to the number of fields in the passed object, and add the not-too-beautifully-named type_nfields to get the field count of instances of a given type (when possible). This is what I've implemented here.
  2. Combine both behaviors in nfields. This is similar to what we have now, except any argument that's a Type will give the field count of its instances, or an error if the type is too abstract. The downside is that there'd be no function that would tell you a UnionAll has 2 fields and a DataType has 16 fields. Granted, that is seldom needed, but it's still a bit confusing.
    EDIT:
  3. Have nfields accept only types (similar to fieldtype), and use nfields(typeof(x)) for instances.

sizeof has a similar problem, but in practice it's not nearly as bad since it's not useful to know the size in bytes of a Type. Therefore I think sizeof(::Type) can be safely interpreted as being about the type's instances. Here I just added some more errors, and allowed sizeof(::Symbol) since we allow write on symbols.

Other than that there is some simple wrapping of type field access for code in src/.

@JeffBezanson JeffBezanson added the breaking This change will break code label Jun 13, 2017
base/exports.jl Outdated
@@ -994,6 +994,7 @@ export
typeintersect,
typejoin,
widen,
type_nfields,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be documented if exported

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you're open to some name bikeshedding, perhaps ntypefields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I could very much use name suggestions, if folks prefer option 1.

@ararslan ararslan added the needs docs Documentation for this change is required label Jun 13, 2017
@iamed2
Copy link
Contributor

iamed2 commented Jun 13, 2017

Another option: have nfields only take types, and only return the number of fields of instances of that type. Then to get the number of fields of the object passed in you would call nfields∘typeof.

@iamed2
Copy link
Contributor

iamed2 commented Jun 13, 2017

To expand on why I think that is better:

I think that nfields to get number of fields of the object passed in makes it seem like the number of fields is a property of the object itself, rather than its Julia representation.

struct Cat
    cute::Bool

    Cat() = Cat(true)
end

Cats (EDIT: the actual animals) don't have fields, but the Julia type Cat does.

@JeffBezanson
Copy link
Member Author

Yes, that's also a good option. The only (very minor) issue with it is that we couldn't have types with variable numbers of fields. We want to have some kind of support for variable-size objects eventually. Now, I believe @yuyichao has a design that uses a fixed number of fields, but where some fields can have variable size, so we don't necessarily need variable field counts. But it's something to consider.

@ararslan
Copy link
Member

Cats don't have fields

The instances do have fields, otherwise you couldn't access their fields as Cat().cute, for example. (Which is universally true, btw. 😉) That said, presumably the number of fields in an instance of a type is always equal to the number of fields of the type, so the distinction seems unimportant.

@iamed2
Copy link
Contributor

iamed2 commented Jun 13, 2017

Sorry, I realize that was unclear, I meant actual cats not Cats.

@JeffBezanson
Copy link
Member Author

fieldname, fieldnames, and fieldoffset have similar issues. All could be made type-only functions, except fieldoffset if objects can have internal variable-size fields. Maybe we can cross that bridge when we get there.

@JeffBezanson
Copy link
Member Author

Hmm, there's a bit of a naming scheme there. Maybe the type-only version of nfields should be renamed fieldcount to go with those other functions?

@ararslan
Copy link
Member

+1, I was actually going to suggest fieldcount earlier but I forgot

@JeffBezanson JeffBezanson force-pushed the jb/nfields branch 2 times, most recently from 0bf4ef4 to 2d6c072 Compare June 14, 2017 02:45
@JeffBezanson
Copy link
Member Author

OK, I have used fieldcount for the type function. For now I've kept nfields for instances. It is very very heavily used and is pretty important to inference, so if we want to fully deprecate it to fieldcount(typeof(x)) that will take a bunch more work.

@JeffBezanson JeffBezanson added needs news A NEWS entry is required for this change and removed needs docs Documentation for this change is required labels Jun 14, 2017
@@ -983,6 +983,7 @@ export
fieldoffset,
fieldname,
fieldnames,
fieldcount,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be added to stdlib doc index

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -153,7 +153,7 @@ end

function show(io::IO, ::MIME"text/plain", opt::JLOptions)
println(io, "JLOptions(")
fields = fieldnames(opt)
fields = fieldnames(JLOptions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminding me that we have two nearly identical versions of this method...

@JeffBezanson JeffBezanson added deprecation This change introduces or involves a deprecation and removed needs news A NEWS entry is required for this change labels Jun 16, 2017
if isType(x)
isleaftype(x.parameters[1]) && return Const(nfields(x.parameters[1]))
# TODO: remove with deprecation in builtins.c for nfields(::Type)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a note in deprecated.jl or this will get missed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@tkelman
Copy link
Contributor

tkelman commented Jun 30, 2017

did news addition get lost in a rebase?

@JeffBezanson
Copy link
Member Author

Yep, looks like it. Found the missing commit locally.

Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

"""
function fieldcount(t::ANY)
if t isa UnionAll || t isa Union
p = ccall(:jl_argument_datatype, Ptr{Void}, (Any,), t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be cleaner to return the Julia sentinel (nothing) rather than the C sentinel

@@ -265,7 +259,7 @@ false
```
"""
isimmutable(x::ANY) = (@_pure_meta; (isa(x,Tuple) || !typeof(x).mutable))
isstructtype(t::DataType) = (@_pure_meta; nfields(t) != 0 || (t.size==0 && !t.abstract))
isstructtype(t::DataType) = (@_pure_meta; length(t.types) != 0 || (t.size==0 && !t.abstract))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the C definition has this same bug

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better definition is probably !abstract && !is_primitive_type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would certainly help with reviewing how those definitions intersect :)

base/show.jl Outdated
println(io)
print(io, indent, " ", fields[idx], "::")
print(tvar_io, fieldtypes[idx])
if !x.abstract
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can move this to the if check on L1253 with the rest of the similar tests

@@ -8,6 +8,7 @@ struct SummarySize
chargeall::Any
end

_nfields(x::ANY) = length(typeof(x).types)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also remove along with deprecation?

src/builtins.c Outdated
// names===() and size > 0 => bitstype, size always known
if (dx->abstract || !jl_is_leaf_type(x))
jl_error("type does not have a fixed size");
if (!jl_is_primitivetype(dx)) { // size of a primitive type is always known
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is redundant with testing dx->layout

@@ -614,12 +614,14 @@ add_tfunc(Core.sizeof, 1, 1,
x !== DataType && isleaftype(x) && return _const_sizeof(x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isType(x) && return _const_sizeof(x.parameters[1])

This case isn't valid unless we define that T == S implies sizeof(T) === sizeof(S), which currently is not true. Either we need to use isConstType here (such that we know T == S implies T === S) or work out all of the equivalence classes that could occur (tempting, since we seem to be really close, but probably difficult to get right)

src/julia.h Outdated
@@ -928,7 +935,7 @@ STATIC_INLINE int jl_is_datatype_singleton(jl_datatype_t *d)
STATIC_INLINE int jl_is_datatype_make_singleton(jl_datatype_t *d)
{
return (!d->abstract && jl_datatype_size(d) == 0 && d != jl_sym_type && d->name != jl_array_typename &&
d->uid != 0 && (d->name->names == jl_emptysvec || !d->mutabl));
d->uid != 0 && (jl_datatype_nfields(d) == 0 || !d->mutabl));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jl_datatype_nfields needs to examine layout, so it's less likely to be valid to use this predicate after this change (wasn't really valid before anyways though). Lets sink it into jltypes.c so that it isn't exported anymore.

@test_throws ErrorException sizeof(Union{Complex64,Complex128})
@test_throws ErrorException sizeof(Union{Int8,UInt8})
@test_throws ErrorException sizeof(AbstractArray)
@test_throws ErrorException sizeof(Real)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use

@test_throws(ErrorException("argument is an abstract type; size is indeterminate"),
             sizeof(Real))

to make sure we aren't just throwing some random error by accident

@JeffBezanson
Copy link
Member Author

@vtjnash Good suggestions. I implemented all of them, and also simplified the type instantiation code a bit. We were handling allocating singleton instances in about 3 places; I moved it all to jl_compute_field_offsets.

- deprecate `fieldnames(v)` to disambiguate whether it operates on
  types or their instances.
- similarly, separate `nfields` for instances and `fieldcount` for types
- more errors for abstract types in `sizeof`
- allow `sizeof(::Symbol)`
- use more abstract type property access in src/
- consolidate make_singleton logic to one place
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants